Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak rockspec to pass CI again #444

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

alerque
Copy link
Member

@alerque alerque commented Aug 24, 2022

A while back @leafo fixed some stuff in the luarocks GH Action we are using to setup containers to test our rockspec builds. With those fixes in place I've been able to re-enable the matrix that checks our builds against multiple luarocks versions. For a long time we had only been testing 3.8.0, now I'm doing an assortment of the newest, the one found on Ubuntu LTS, and the oldest supported.

This has turned up the fact that the checkout branch declaration we use was broken in LuaRocks 3.1.3 due to an upstream bug. It doesn't play nice with automatically figuring out whether something is a tag or a branch and ends up trying to concatenate a string to nil. This was fixed in Luarocks, but that version has quite a bit of market penetration so there is some sense in testing with it.

This uses an uglier but effective workaround to get the same work done without leaving 3.1.3 in the cold.

@alerque
Copy link
Member Author

alerque commented Aug 24, 2022

Note this project isn't (yet) using our reusable workflows for rockspec testing hence the failures on 3.1.3 aren't showing up here, I'm just copying this from lessons learned testing other projects. If you want I want I can bring in the rockspec testing and auto-publish stuff to this project too.

@alerque
Copy link
Member Author

alerque commented Aug 25, 2022

If you don't like the coding style I just noticed copas has a different workaround:

https://github.com/lunarmodules/copas/blob/master/copas-cvs-6.rockspec#L12-L13

Slightly more verbose but also easier to grok.

@Tieske
Copy link
Member

Tieske commented Aug 25, 2022

If you don't like the coding style I just noticed copas has a different workaround:

https://github.com/lunarmodules/copas/blob/master/copas-cvs-6.rockspec#L12-L13

Slightly more verbose but also easier to grok.

This is the one I use these days, so this definitely has my preference

@Tieske
Copy link
Member

Tieske commented Aug 25, 2022

also I tried to switch to "dev" rockspecs a while ago, because it seemed the more obvious name. But it turned out it gave me some issues with LuaRocks, it seems not all labels are treated equal; dev/scm/cvs.

Maybe @hishamhm has some background on this?

@alerque
Copy link
Member Author

alerque commented Aug 26, 2022

I'm confused on the dev/scm/cvs bit myself, I keep wondering if they really are treated equal. I'd be happy to push projects in one directory of the other (obviously not so much the third) but only if there really is a reason to do so.

And sure I'll tweak the rockspec code styling.

@alerque
Copy link
Member Author

alerque commented Aug 26, 2022

Also (not talking to you specifically just the ecosystem in general) the practice of rockrel bumping makes no sense to me on dev/scm/cvs rockspecs, I wish people would stop doing that.

@alerque
Copy link
Member Author

alerque commented Aug 26, 2022

As long as I was at it using a more readable workaround, I tweaked the whole thing a bit for what I consider better readability. Let me know if you don't like the style changes.

@alerque
Copy link
Member Author

alerque commented Aug 26, 2022

As long as we are mucking with rockspecs we might as well test them. I added our modular rockspec build checks. We'll have to check results in my fork until after the PR merge I think.

I did not include the automatic deploy part that publishes rockspecs after checking them, just the test build section.

Setting source.branch works for most cases, but when the branch is
actually a tag and not a branch then a Luarocks 3.1.3 bug rears
and tries to concatenate a nil. This dodges that bullet by setting
source.tag explicitly to not trigger the branch_or_tag concatenation.

All the other changes are cosmetic and for readability/maintainability.
@alerque
Copy link
Member Author

alerque commented Aug 26, 2022

Rockspec build checks ran here.

@Tieske
Copy link
Member

Tieske commented Aug 30, 2022

Also (not talking to you specifically just the ecosystem in general) the practice of rockrel bumping makes no sense to me on dev/scm/cvs rockspecs, I wish people would stop doing that.

I actually thought about creating a GHA that actually checks and barfs if you do 😄

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very opinionated....

local github_repo_name = package_name
local git_checkout = package_version == "dev" and "master" or package_version

package = "penlight"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the vertical alignment of the "local"s, aesthetically. Despite this is just copying one variable into another.

(hence also the 2 lines white-space below to accentuate the vertical alignment above)


rockspec_format = "3.0"
package = package_name
version = package_version .. "-" .. rockspec_revision
version = ("%s-%s"):format(rock_version, rock_release)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the concatenations are easier to read for non-Lua folks than the format-placeholder approach.

Comment on lines +12 to +13
branch = rock_version == "dev" and "master" or nil,
tag = rock_version ~= "dev" and rock_version or nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this I like, I have switched other rockspecs to this as well. Wondering if it is worthwhile to remove the hardcoded "master" here and add a local at the top?

local default_branch = "master"

@hishamhm
Copy link
Member

hishamhm commented Aug 30, 2022

also I tried to switch to "dev" rockspecs a while ago, because it seemed the more obvious name. But it turned out it gave me some issues with LuaRocks, it seems not all labels are treated equal; dev/scm/cvs.

Maybe @hishamhm has some background on this?

I'm confused on the dev/scm/cvs bit myself, I keep wondering if they really are treated equal.

They're almost equal; In the presence of both in a server, dev takes precedence.

The background story is that in the olden CVS days they started as cvs; then we had the Version Control Wars when we actually had to support multiple contenders and I picked the name scm as a neutral one; but then the luarocks.org website came along and started to refer to them as "dev" in the UI, so I added support to dev to match the website and --dev in the CLI, and in the manifest-sorting order I gave it a larger priority than scm (which already had a larger prio than cvs).

TL;DR: just use dev everywhere, it should be fine to just delete any other ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants